Skip to content

feat(webpack-cli): add no-mode flag#1276

Merged
ematipico merged 2 commits into
webpack:nextfrom
snitin315:feat/no-mode-flag
Mar 3, 2020
Merged

feat(webpack-cli): add no-mode flag#1276
ematipico merged 2 commits into
webpack:nextfrom
snitin315:feat/no-mode-flag

Conversation

@snitin315

@snitin315 snitin315 commented Feb 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

feature - add no-mode flag

Did you add tests for your changes?
Work in progress.

If relevant, did you update the documentation?
Yes

Summary

Refers #1240

Refers #1012

Does this PR introduce a breaking change?

Other information

@snitin315
snitin315 requested a review from a team as a code owner February 27, 2020 16:33
ematipico
ematipico suggested changes Feb 27, 2020
View reviewed changes

@ematipico ematipico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test, update the documentation and the snapshot tests

ematipico
ematipico suggested changes Feb 28, 2020
View reviewed changes

@ematipico ematipico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the way --no-mode is retrieved

Comment thread packages/webpack-cli/lib/groups/ZeroConfigGroup.js Outdated
ematipico
ematipico suggested changes Feb 28, 2020
View reviewed changes

@ematipico ematipico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some other refactor can be done

Comment thread packages/webpack-cli/__tests__/ZeroConfigGroup.test.js Outdated
Comment thread packages/webpack-cli/lib/groups/ZeroConfigGroup.js Outdated
rishabh3112
rishabh3112 suggested changes Feb 28, 2020
View reviewed changes

@rishabh3112 rishabh3112 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting efforts!
Try to open PRs as draft pull requests in case you feel they are work in progress in future 😉.

@snitin315

snitin315 commented Feb 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

@ematipico @rishabh3112 , Yes there is a bug --mode is accepting any value and outputs development build. I can pass --mode=abcd , and it produces the build.

It should accept only production , development or none

Shoul I open a seperate issue for this? can you please verify?

@ematipico

ematipico commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

Argument validation will be part of another story. Don't worry about it.
Just focus on your feature for now.

@snitin315 snitin315 changed the title feat(webpack-cli): add no-mode flag - [WIP] feat(webpack-cli): add no-mode flag Feb 29, 2020
@snitin315
snitin315 force-pushed the feat/no-mode-flag branch from 81418fc to 629b16f Compare March 1, 2020 03:20
@ematipico

ematipico commented Mar 1, 2020

Copy link
Copy Markdown
Contributor

Could you rebase your branch please?

@snitin315

snitin315 commented Mar 1, 2020

Copy link
Copy Markdown
Member Author

@ematipico already done 👍

ematipico
ematipico suggested changes Mar 1, 2020
View reviewed changes

@ematipico ematipico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also have to update the snapshot tests

snitin315
snitin315 commented Mar 1, 2020
View reviewed changes
Comment thread test/help/__snapshots__/help-single-arg.test.js.snap
@snitin315
snitin315 force-pushed the feat/no-mode-flag branch from 629b16f to 6a45b54 Compare March 2, 2020 15:42
@webpack-bot

webpack-bot commented Mar 2, 2020

Copy link
Copy Markdown

@snitin315 Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

@snitin315

snitin315 commented Mar 3, 2020

Copy link
Copy Markdown
Member Author

@ematipico can you help me fix the CI , dont know why scss test is failing.

@ematipico

ematipico commented Mar 3, 2020

Copy link
Copy Markdown
Contributor

Please add a it.skip to json.test.js. That file is the culprit

@webpack-bot

webpack-bot commented Mar 3, 2020

Copy link
Copy Markdown

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@snitin315
snitin315 requested a review from ematipico March 3, 2020 15:02
ematipico
ematipico approved these changes Mar 3, 2020
View reviewed changes

@ematipico ematipico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ematipico
ematipico merged commit a069d73 into webpack:next Mar 3, 2020
@snitin315
snitin315 deleted the feat/no-mode-flag branch March 3, 2020 15:11
@snitin315 snitin315 mentioned this pull request Mar 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rishabh3112 rishabh3112 Awaiting requested review from rishabh3112
1 more reviewer
@ematipico ematipico ematipico approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR: CI-ok PR: reviewed-approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@snitin315 @ematipico @webpack-bot @rishabh3112